MLX Cleanup #187
MLX Cleanup #187
Conversation
- updated readme - updated makefile
| xcodebuild clean build-for-testing -scheme ${{ matrix.run-config['scheme'] }} -destination '${{ matrix.run-config['clean-destination'] }}' -skipPackagePluginValidation | xcpretty | ||
| xcodebuild test -only-testing WhisperKitTests/UnitTests -scheme ${{ matrix.run-config['scheme'] }} -destination '${{ matrix.run-config['test-destination'] }}' -skipPackagePluginValidation | ||
| xcodebuild clean build-for-testing test \ | ||
| ${{ matrix.run-config['test-cases'] }} \ | ||
| -scheme ${{ matrix.run-config['scheme'] }} \ | ||
| -destination '${{ matrix.run-config['test-destination'] }}' \ | ||
| -skipPackagePluginValidation |
There was a problem hiding this comment.
this is merged into single command xcodebuild clean build-for-testing test
| @xcodebuild CLANG_ENABLE_CODE_COVERAGE=NO VALID_ARCHS=arm64 clean build \ | ||
| -configuration Release \ | ||
| -scheme whisperkit-Package \ | ||
| -destination generic/platform=macOS \ | ||
| -derivedDataPath .build/.xcodebuild/ \ | ||
| -clonedSourcePackagesDirPath .build/ \ | ||
| -skipPackagePluginValidation |
There was a problem hiding this comment.
we have to use xcodebuild because of mlx
| @swift build -c release --product whisperkit-cli | ||
| @xcodebuild CLANG_ENABLE_CODE_COVERAGE=NO VALID_ARCHS=arm64 clean build \ | ||
| -configuration Release \ | ||
| -scheme whisperkit-cli \ | ||
| -destination generic/platform=macOS \ | ||
| -derivedDataPath .build/.xcodebuild/ \ | ||
| -clonedSourcePackagesDirPath .build/ \ | ||
| -skipPackagePluginValidation | ||
|
|
||
|
|
||
| test: | ||
| @echo "Running tests..." | ||
| @swift test -v | ||
| @xcodebuild clean build-for-testing test \ | ||
| -scheme whisperkit-Package \ | ||
| -only-testing WhisperKitMLXTests/MLXUnitTests \ | ||
| -only-testing WhisperKitTests/UnitTests \ | ||
| -destination 'platform=macOS,arch=arm64' \ | ||
| -skipPackagePluginValidation |
There was a problem hiding this comment.
we have to use xcodebuild because of mlx
Package.swift
Outdated
| .package(url: "https://github.com/ml-explore/mlx-swift", branch: "main"), | ||
| .package(url: "https://github.com/ml-explore/mlx-swift", revision: "d6d9472da5bf7ec2654e8914bd1d15622f45b6a9"), |
There was a problem hiding this comment.
the latest version of mlx 0.16.0 breaks the implementation, let's pin it to the last commit before the breaking one, I'll have to talk to mlx team about the issue
There was a problem hiding this comment.
fixed, solution provided here ml-explore/mlx-swift#117
README.md
Outdated
| Task { | ||
| let pipe = try? await WhisperKit() | ||
| let transcription = try? await pipe!.transcribe(audioPath: "path/to/your/audio.{wav,mp3,m4a,flac}")?.text | ||
| print(transcription) | ||
| } | ||
| let pipe = try await WhisperKit() | ||
| // Transcribe the audio file | ||
| let transcription = try await pipe.transcribe(audioPath: "path/to/your/audio.{wav,mp3,m4a,flac}")?.text | ||
| // Print the transcription | ||
| print(transcription) |
There was a problem hiding this comment.
removed not needed Task, decided to remove ? from try ?. When the potential user does copy paste from the readme I think it's better when the error is not ignored
| ``` | ||
|
|
||
| ### Model Selection | ||
|
|
||
| WhisperKit automatically downloads the recommended model for the device if not specified. You can also select a specific model by passing in the model name: | ||
| You have to specify the model by passing the model name: |
There was a problem hiding this comment.
Right now user has to pass either model and / or mlxModel, depending whether mlx is used or not
| public func prewarmModels() async throws { | ||
| try await loadModels(prewarmMode: true) | ||
| } | ||
|
|
||
| public func loadModels( | ||
| prewarmMode: Bool = false | ||
| ) async throws { | ||
| modelState = prewarmMode ? .prewarming : .loading | ||
| assert(modelFolder != nil || mlxModelFolder != nil, "Please specify `modelFolder` or `mlxModelFolder`") |
There was a problem hiding this comment.
Will this crash on prod builds? If so it should throw instead of crash.
There was a problem hiding this comment.
it's not going to crash on a prod build
| @@ -211,114 +228,80 @@ open class WhisperKit { | |||
| } | |||
| } | |||
|
|
|||
| /// Sets up the model folder either from a local path or by downloading from a repository. | |||
| public func setupModels( | |||
There was a problem hiding this comment.
Is this deleted for a specific reason? Would be preferred to keep with with some adjustments since it was previously a public function and may be getting called elsewhere.
There was a problem hiding this comment.
previously setupModels would set modelFolder property. Right now we have to set either modelFolder or / and mlxModelFolder depending on the pipeline setup. In this context this function doesn't make sense any more
|
Test passing ✅ |
In this PR:
added new params to
WhisperKit:mlxModelif specified, it will be used to specify which mlx model to downloadmlxModelRepoif specified, it will be used to specify the repo where the mlx model is storedmlxModelFolderif specified, it will point to the local folder where the mlx model is storedadded new options to
CLIArguments:mlxModel,mlxModelPathsee the explanation abovefeatureExtractorType,audioEncoderType,textDecoderTypeto specify which type (coreml or or mlx) should be used for given pipeline stepupdated readme to reflect the changes above
updated makefile to reflect the changes above
simplified CI unit test
To do:
xcodebuildinstead ofswift build